Added "ACL LOG" command new in Redis 6 RC2 #1307

jiekun · 2020-03-15T13:08:46Z

Pull Request check-list

Please make sure to review and check all of these items:

Does $ tox pass with this change (including linting)?
Does travis tests pass with this change (enable it first in your forked repo and wait for the travis build to finish)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Hi. First of all thanks for checking this.
There is an "ACL LOG" command added in Redis 6 RC2. I tried to add it to redis-py.

The "ACL LOG" command supports two kind of arguments: ACL LOG [<count> | RESET]. For <count>, it returns a acl log list. For RESET, it returns 'OK'. So I parsed it into:

The original response for 'ACL LOG':
[[b'count', 1, b'reason', b'command', b'context', b'toplevel', b'object', b'set', b'username', b'duck', b'age-seconds', b'1.958', b'client-info', b'id=3180 addr=127.0.0.1:59566 fd=9 name= age=105 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=30 qbuf-free=32738 obl=0 oll=0 omem=0 events=r cmd=set user=duck']]

The redis-py output for 'ACL LOG':
[{'count': 1, 'reason': 'command', 'context': 'toplevel', 'object': 'set', 'username': 'duck', 'age-seconds': 1.958, 'client-info': {'id': 3180, 'addr': '127.0.0.1:59566', 'fd': '9', 'name': '', 'age': 105, 'idle': 0, 'flags': 'N', 'db': 0, 'sub': 0, 'psub': 0, 'multi': -1, 'qbuf': 30, 'qbuf-free': 32738, 'obl': 0, 'oll': 0, 'omem': 0, 'events': 'r', 'cmd': 'set', 'user': 'duck'}}]

-------------------- break --------------------

The original response for 'ACL LOG RESET':
b'OK'

The redis-py output for 'ACL LOG RESET':
True

Note that the wget link in .travis.yml is changed as well since the RC1 does not have a LOG sub-command for ACL.

Please take a look. It's my first PR for open-source project. Thank you.

codecov-io · 2020-03-15T13:15:51Z

Codecov Report

Merging #1307 into master will increase coverage by 0.09%.
The diff coverage is 92.98%.

@@            Coverage Diff            @@
##           master   #1307      +/-   ##
=========================================
+ Coverage   92.81%   92.9%   +0.09%     
=========================================
  Files          19      19              
  Lines        6526    6583      +57     
=========================================
+ Hits         6057    6116      +59     
+ Misses        469     467       -2

Impacted Files	Coverage Δ
tests/test_commands.py	`99.94% <100%> (ø)`	⬆️
redis/client.py	`86.32% <88.57%> (+0.04%)`	⬆️
redis/connection.py	`82.82% <0%> (+0.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2f470e...e53a8c0. Read the comment docs.

andymccurdy · 2020-03-15T16:24:51Z

I skimmed the code and this initially looks well done. Thanks!

Is there any documentation for ACL LOG on redis.io or elsewhere? I looked at the ACL section of the redis.io website and found no mention of this.

itamarhaber · 2020-03-15T16:29:21Z

It has yet to be documented

jiekun · 2020-03-15T16:36:38Z

Is there any documentation for ACL LOG on redis.io or elsewhere?

Yes the ACL doc is not updated.
This pull request is based on Antirez's videos and source code and some redis-cli test myself.

The return type may change to boolean, base on what argument is used. I am thinking about splitting them into two methods like `acl_log()` and `acl_log_reset()`. But that will not match the Redis command usage which is `ACL LOG [<count> | RESET]`.

jiekun · 2020-03-15T17:02:48Z

The return type may change from list to boolean, based on what argument is used. I am thinking about splitting them into two methods like acl_log() and acl_log_reset().
But that will not match the Redis command usage which is ACL LOG [<count> | RESET].

jiekun · 2020-05-06T12:38:22Z

Hi. Any idea about this one? @andymccurdy
https://redis.io/commands/acl-log

RoeyPrat · 2020-06-29T20:09:47Z

The return type may change from list to boolean, based on what argument is used. I am thinking about splitting them into two methods like acl_log() and acl_log_reset().
But that will not match the Redis command usage which is ACL LOG [<count> | RESET].

Hi @2014BDuck, great work!
I think it would be better to split the method into two methods as you've suggested.
acl_log and acl_log_reset have different use cases, as well as returning different types.

Also note you can drop the commit to change redis version in travis, as we are now using version 6.0.5 on master.

jiekun · 2020-07-02T10:53:56Z

The return type may change from list to boolean, based on what argument is used. I am thinking about splitting them into two methods like acl_log() and acl_log_reset().
But that will not match the Redis command usage which is ACL LOG [<count> | RESET].

Hi @2014BDuck, great work!
I think it would be better to split the method into two methods as you've suggested.
acl_log and acl_log_reset have different use cases, as well as returning different types.

Also note you can drop the commit to change redis version in travis, as we are now using version 6.0.5 on master.

For 6.0.5: Cool let me merge the master first so that old commit will be covered
For seperated it into 2 method: OK Will do

codecov-commenter · 2020-07-06T06:22:07Z

Codecov Report

Merging #1307 into master will decrease coverage by 7.00%.
The diff coverage is 87.71%.

@@            Coverage Diff             @@
##           master    #1307      +/-   ##
==========================================
- Coverage   92.76%   85.76%   -7.01%     
==========================================
  Files          20        7      -13     
  Lines        6581     3049    -3532     
==========================================
- Hits         6105     2615    -3490     
+ Misses        476      434      -42

Impacted Files	Coverage Δ
redis/client.py	`86.13% <86.27%> (+0.04%)`	⬆️
redis/connection.py	`82.18% <100.00%> (+0.43%)`	⬆️
redis/exceptions.py	`100.00% <100.00%> (ø)`
redis/_compat.py	`86.99% <0.00%> (-0.11%)`	⬇️
redis/lock.py	`100.00% <0.00%> (ø)`
tests/test_monitor.py
tests/test_scripting.py
redis/__init__.py
tests/test_connection.py
tests/test_pipeline.py
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20ec5d7...7995723. Read the comment docs.

andymccurdy · 2020-07-06T16:40:13Z

@2014BDuck Awesome, thanks. Ping me when you get this split into two separate functions and we'll get it merged.

jiekun · 2020-07-08T05:37:27Z

@andymccurdy Hi ptal one more time. Thanks

andymccurdy

This is really close. Thanks for splitting up the command. A few inline notes below and then we'll get this merged. Thanks!

andymccurdy · 2020-07-08T22:53:20Z

tests/test_commands.py

+        r_test.set("cache:0", 1)
+        r_test.get("cache:0")
+        # Invalid key
+        try:


This should be:

with pytest.raises(exceptions.NoPermissionError): r_test.get('violated_cache:0')

Done. Thanks.

andymccurdy · 2020-07-08T22:53:53Z

tests/test_commands.py

+        except exceptions.NoPermissionError:
+            pass
+        # Invalid operation
+        try:


Same as above:

with pytest.raises(exceptions.NoPermissionError): r_test.hset('cache:0', 'hkey', 'hval')

Done. Thanks.

andymccurdy · 2020-07-08T22:57:25Z

tests/test_commands.py

+            pass
+
+        assert len(r.acl_log()) == 2
+        assert len(r.acl_log(count=1)) == 1


It would be nice to assert that acl_log() returns a dict and that a common set of keys exist. Additionally we should check that the 'click-info' value is a dict as well.

Added few return type assertion now.

andymccurdy · 2020-07-08T23:01:06Z

tests/test_commands.py

+                      keys=['cache:*'], nopass=True)
+        r.acl_log_reset()
+
+        r_test = redis.Redis(host='localhost', port=6379, db=9,


We don't want to hardcode connection info in a test. Instead, use the _get_client function from conftest.py. Something like:

r_test = _get_client(redis.Redis, request, username=username)

Yep please check the comment below

jiekun · 2020-07-11T03:52:09Z

Hi @andymccurdy
I've added some the "assert" cases and changed few cases into "with" pattern, which is more accurate for the testing. Thanks for your guidence & learned a lot.

For the hardcoded connection:

r_test = redis.Redis(host='localhost', port=6379, db=9...

Yep it's not pretty. I tried to find out the correct pattern to new a different client before writting it but haven't found any example.

The get_client way r_test = _get_client(redis.Redis, request, username=username) : Seems like the deeper codes won't use the username variable in **kwargs but from the passed in request object:

# This method passed redus_url string grabbed from request obj. And passed to "from_url" class method.
def _get_client(cls, request, single_connection_client=True, **kwargs):
    redis_url = request.config.getoption("--redis-url")
    client = cls.from_url(redis_url, **kwargs)
    .....


# This method will parse url and use the username in url.
    def from_url(cls, url, db=None, decode_components=False, **kwargs):
        url = urlparse(url)
        url_options = {}

        ...
        # Here. The username is token directly from the url we passed in.
        if decode_components:
            username = unquote(url.username) if url.username else None
            ...
        else:
            username = url.username or None
            ...

So it's not working using r_test = _get_client(redis.Redis, request, username=username). I am thinking about:

Altering the attribute of request obj ? like request.config.option.redis_url = ''redis://xxx:localhost:6379/9'. But I think it looks more weird than building a new client with redis.Redis(host='localhost', port=6379, db=9)
Altering the deeper method from_url using username in **kwargs if existed. Otherwise following the current pattern.

Actually both of them sound worse from my point of view. Would like to hear more advice from your side. Many thanks.

jiekun · 2020-07-25T08:35:33Z

@andymccurdy Hi. May I request for a review and your suggestion again?

Two things after I merged the current master branch.

The coverage test is not passed due to...? Looks like it's not caused by my changes? Should I re-open a new PR or any suggestion?
The hardcoded connection. If we need to pass the username as connection param, it would require additional changes on couple methods, which is not graceful enough from my point of view.

andymccurdy · 2020-08-16T00:38:55Z

@2014BDuck I fixed this in the acl-log branch here: https://github.com/andymccurdy/redis-py/tree/acl-log

I believe that branch is good to go. Let me know if you see anything off. If you want to merge it back into this PR I'll then merge your PR to master. Alternatively I can just merge the acl-log branch to master. Let me know what you prefer.

jiekun · 2020-08-19T09:27:46Z

Alternatively I can just merge the acl-log branch to master

@andymccurdy Yes please.

And sorry for wasting your time and effort on guiding. Very appreciated.

andymccurdy · 2020-08-19T17:48:53Z

Merged. Thanks!

2014bduck added 4 commits March 15, 2020 19:19

AddED ACL Log command in Redis 6 RC2

6f3d00d

Fixed test case for acl log

09c07c2

Changed travis wget from rc1 to rc2

38a74a2

Added doc for acl_log

6e212d1

jiekun changed the title ~~Added "ACL LOG" command in Redis 6 RC2~~ Added "ACL LOG" command new in Redis 6 RC2 Mar 15, 2020

2014bduck added 2 commits March 16, 2020 00:58

Updated doc for acl_log method

ee25e2f

The return type may change to boolean, base on what argument is used. I am thinking about splitting them into two methods like `acl_log()` and `acl_log_reset()`. But that will not match the Redis command usage which is `ACL LOG [<count> | RESET]`.

typo

e53a8c0

Synced with upstream master

85254af

RoeyPrat assigned jiekun Jul 6, 2020

Changed acl_log(count, reset) into acl_log(count) and acl_log_reset()

0a37ea8

RoeyPrat requested a review from andymccurdy July 8, 2020 08:46

andymccurdy suggested changes Jul 8, 2020

View reviewed changes

Added test case assert for ACL LOG command

47d8b03

jiekun requested a review from andymccurdy July 12, 2020 03:55

2014bduck added 3 commits July 14, 2020 13:18

Merge branch 'master' into master

d978efd

Merge branch 'master' of https://github.com/andymccurdy/redis-py

0e63f21

Fixed hardcoded redis host to 'master'

7995723

andymccurdy closed this in b80d423 Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added "ACL LOG" command new in Redis 6 RC2 #1307

Added "ACL LOG" command new in Redis 6 RC2 #1307

jiekun commented Mar 15, 2020

codecov-io commented Mar 15, 2020 •

edited

Loading

andymccurdy commented Mar 15, 2020

itamarhaber commented Mar 15, 2020

jiekun commented Mar 15, 2020

jiekun commented Mar 15, 2020

jiekun commented May 6, 2020 •

edited

Loading

RoeyPrat commented Jun 29, 2020

jiekun commented Jul 2, 2020

codecov-commenter commented Jul 6, 2020 •

edited

Loading

andymccurdy commented Jul 6, 2020

jiekun commented Jul 8, 2020

andymccurdy left a comment

andymccurdy Jul 8, 2020

jiekun Jul 11, 2020

andymccurdy Jul 8, 2020

jiekun Jul 11, 2020

andymccurdy Jul 8, 2020

jiekun Jul 11, 2020

andymccurdy Jul 8, 2020

jiekun Jul 11, 2020

jiekun commented Jul 11, 2020

jiekun commented Jul 25, 2020

andymccurdy commented Aug 16, 2020

jiekun commented Aug 19, 2020 •

edited

Loading

andymccurdy commented Aug 19, 2020

Added "ACL LOG" command new in Redis 6 RC2 #1307

Added "ACL LOG" command new in Redis 6 RC2 #1307

Conversation

jiekun commented Mar 15, 2020

Pull Request check-list

Description of change

codecov-io commented Mar 15, 2020 • edited Loading

Codecov Report

andymccurdy commented Mar 15, 2020

itamarhaber commented Mar 15, 2020

jiekun commented Mar 15, 2020

jiekun commented Mar 15, 2020

jiekun commented May 6, 2020 • edited Loading

RoeyPrat commented Jun 29, 2020

jiekun commented Jul 2, 2020

codecov-commenter commented Jul 6, 2020 • edited Loading

Codecov Report

andymccurdy commented Jul 6, 2020

jiekun commented Jul 8, 2020

andymccurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiekun commented Jul 11, 2020

jiekun commented Jul 25, 2020

andymccurdy commented Aug 16, 2020

jiekun commented Aug 19, 2020 • edited Loading

andymccurdy commented Aug 19, 2020

codecov-io commented Mar 15, 2020 •

edited

Loading

jiekun commented May 6, 2020 •

edited

Loading

codecov-commenter commented Jul 6, 2020 •

edited

Loading

jiekun commented Aug 19, 2020 •

edited

Loading